GitHub Issue #1130: Metrics to track R & Python package usage - #7881
GitHub Issue #1130: Metrics to track R & Python package usage#7881cnathe wants to merge 15 commits into
Conversation
…ecordPackageUsage, and recordSuccessfulRun overrides
…ng via ScriptPackageUsageTracker.record()
…cording via ScriptPackageUsageTracker.record()
…unt of number of times a package was used
|
@labkey-jeckels @labkey-matthewb note that this approach doesn't track anything for Jupyter reports or those that run via Rserve. Thoughts? |
labkey-jeckels
left a comment
labkey-jeckels
left a comment
There was a problem hiding this comment.
I'm not worried about Jupyter since we only have reports on our own cloud servers, not customer instances.
https://www.labkey.org/_mothership/wiki-page.view?name=adhocReport&ids=32119&serverType=customers
See what you think about the suggestions to inject as a shutdown-type handler.
| if (extensions.isEmpty()) | ||
| throw new ScriptException("There are no file name extensions registered for this ScriptEngine : " + getFactory().getLanguageName()); | ||
|
|
||
| String epilog = getPackageCaptureEpilog(context); |
There was a problem hiding this comment.
I didn't dive deep to fully prove it to myself, but Claude thinks that RserveScriptEngine won't capture the package lists.
There was a problem hiding this comment.
correct. I called this out in the PR comment. Claude does seem to think it wouldn't be too large of a change to get this behavior for RserverScriptEngine, so I'm looking into that now.
There was a problem hiding this comment.
I was able to add the RserveScriptEngine implementation to match. It was overriding the entire eval() method so just needed to poke in the calls to these new methods.
| * (via sys.stdlib_module_names), so no Python entry is needed. | ||
| */ | ||
| private static final Map<String, Set<String>> BASE_PACKAGES = Map.of( | ||
| "r", Set.of("base", "compiler", "datasets", "graphics", "grDevices", "methods", "stats", "utils") |
There was a problem hiding this comment.
Claude thinks that these are missing from the list: grid, tools, parallel, splines, stats4, and tcltk
There was a problem hiding this comment.
I'll add "tools" to the list, but I don't get any of the other ones listed when I open Rstudio locally and run sort(loadedNamespaces()) or when I open a new R report on nightly and run that same comment.
There was a problem hiding this comment.
FYI, I ended up adding that full set to the BASE_PACKAGES list
|
|
||
| // Python appended to the end of a user script to capture the loaded modules (top-level names, excluding the standard | ||
| // library). Wrapped in try/except so a capture failure can never break the script run. | ||
| private static final String PACKAGE_CAPTURE_EPILOG = """ |
There was a problem hiding this comment.
This doesn't handle scripts that exit with success via a sys.ext. Consider using atexit.register() in the prologue instead, or possibly a sitecustomize.py.
https://docs.python.org/3/library/site.html
reg.finalizer(globalenv(), f, onexit = TRUE) might work for R.
There was a problem hiding this comment.
good call. a minor adjustment was made to use a prolog instead of epilog and the atexit / finalizer approach as mentioned
There was a problem hiding this comment.
after doing another claude code review pass on this prepend vs append thing, it has several concerns with the prepend approach: Python script containing from __future__ import ... fail to compil, line numbers for errors in the script will be off, etc. This makes me think we should go back to the append case and start with that to track package usage in the success case as a starting point. I believe that will still get us the information we are after for this issue. Thoughts? @labkey-jeckels
…as its own eval() override)
…kage usage on exit/failure
| def _lk_write_packages(): | ||
| try: | ||
| _lk_stdlib = set(getattr(_lk_sys, 'stdlib_module_names', ())) | ||
| _lk_mods = sorted({m.split('.')[0] for m in list(_lk_sys.modules)} - _lk_stdlib) |
There was a problem hiding this comment.
I don't believe this will give us the most useful output. In the Python ecosystem you can have a package that's installed like pip install lib_foo but imported as import foo, I believe the code here will only ever report foo, when what we really want is lib_foo. We should be able to use the importlib.metadata packages_distributions() mapping to convert the imported names to the library names. See here.
3 participants
Rationale
https://github.com/LabKey/internal-issues/issues/1130
The change instruments ExternalScriptEngine to record which R packages / Python modules scripts load, so the counts get reported to mothership via SimpleMetricsService. It does this by: (1) appending a language-specific "capture epilog" to the end of the user script that writes loaded packages to a sidecar file; (2) reading that sidecar back after the run and incrementing a per-package counter; and (3) adding a PythonScriptEngine subclass plus manager wiring to detect .py engines. All metric work is wrapped so it can never break script execution.
Related Pull Requests
Changes